Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rector: CQ - SimplifyIfReturnBoolRector #4152

Closed
wants to merge 40 commits into from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Aug 12, 2024

@github-actions github-actions bot added environment composer Relates to composer.json ddev labels Aug 12, 2024
@github-actions github-actions bot added Component: PayPal Relates to Mage_Paypal Component: Core Relates to Mage_Core Component: Catalog Relates to Mage_Catalog Component: Cms Relates to Mage_Cms Component: CatalogInventory Relates to Mage_CatalogInventory Component: Checkout Relates to Mage_Checkout Component: lib/Varien Relates to lib/Varien Component: Sales Relates to Mage_Sales Component: Customer Relates to Mage_Customer Component: Eav Relates to Mage_Eav Component: lib/Mage Relates to lib/Mage Component: Adminhtml Relates to Mage_Adminhtml labels Aug 12, 2024
@github-actions github-actions bot added rector and removed composer Relates to composer.json ddev labels Sep 22, 2024
@sreichel
Copy link
Contributor Author

@kiatng can we continue w/o DeMorgan's Law?

@kiatng
Copy link
Contributor

kiatng commented Sep 23, 2024

I cannot give my approval because in my opinion, some changes are worse than the current original code.

@sreichel
Copy link
Contributor Author

sreichel commented Sep 23, 2024

B/c of 6 out of +100 improvements?

Dont get me wrong, i agree with your suggestions, but i dont want to mix it up with automatic fixes.

How about to merge and verify your manual changes again?

@sreichel
Copy link
Contributor Author

... at the other hand ... we can leave it open. With rector added as dependency we can skip it for now.

@kiatng
Copy link
Contributor

kiatng commented Sep 23, 2024

Or submit a PR to improve Rector (which I cannot do).

@sreichel
Copy link
Contributor Author

sreichel commented Sep 23, 2024

I'd really like to do, but i am not so familiar with rectors internal functionality.

I think i can write simple custom rules, but in this case ... there are a lot of possibly combinations of "&&", "||" that makes it difficult to make it safe for a general rule. I guess thats the reason why your issue has been closed.

The current rule works, but - as you have seen - it does only covers some "easy" patterns.

We can add this rule, but it will only cover 2 (?) out of your 6 suggestions.

@fballiano
Copy link
Contributor

this PR makes the code much harder to read, the benefit is only for the machine, not for the humans.

@sreichel
Copy link
Contributor Author

Disagree. Most of the changes look better to me.

  • it safes a lot of lines
  • how often do you look at core code?

@fballiano
Copy link
Contributor

  • how often do you look at core code?

always, since the code documentation doesn't exist

and this is not better, it requires more brain processing time for no reason at all -> more possibility to misunderstand the code
Screenshot 2024-09-24 alle 17 23 41

@sreichel
Copy link
Contributor Author

sreichel commented Sep 24, 2024

It's a negated true/false expression?

So what?

@fballiano
Copy link
Contributor

because I'm stupid I think that a negated OR is far less readable. but better make the code 1 line less but much harder to read so that only geniuses will take the time to read it.

again, the code is not for the machines, it's for the people, it should be understandable easily for the people.

only because rector has that rule it doesn't mean it should be applied.

@sreichel
Copy link
Contributor Author

I did not say you're stupid ... you're not.

I'd like to use rector to unify code.

For core, for extensions ...

@kiatng
Copy link
Contributor

kiatng commented Sep 25, 2024

I need to call out @sreichel comment:

how often do you look at core code?

Even though it is a simple question, it is irrelevant. The question carries with it negative implications, which are not within standards of our CoC:

... we as contributors and maintainers pledge to making participation in our project and our community a harassment-free experience for everyone, regardless of ... level of experience, education, ...

and in particular the second standard:

Being respectful of differing viewpoints and experiences

Note that our CoC was based on v1.4. The latest version is 2.1. I encourage everyone here take the time to read it and think about it. The second standard in this version includes "opinions":

Being respectful of differing opinions, viewpoints, and experiences

I urge all of us use language, English as well as PHP, that complies to the best standards.

@sreichel
Copy link
Contributor Author

sreichel commented Sep 25, 2024

There was no "negative implications"! It was a serious question. If you disagree with that change, it is fine.

However ...

@sreichel sreichel closed this Sep 25, 2024
@sreichel sreichel deleted the rector-2-cq branch September 25, 2024 03:02
@sreichel
Copy link
Contributor Author

I need to call out @sreichel comment:

how often do you look at core code?

Maybe you got me wrong?

This PR would have added 7 negated return statements.

My question was how often you face these seven particular lines during development,

When you serach the codebase for return ! you'll have +700 matches, so it is a very common pattern. Decline to add 7 more b/c of "readablility" sound bit weired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Admin Relates to Mage_Admin Component: Adminhtml Relates to Mage_Adminhtml Component: Api2 Relates to Mage_Api2 Component: Authorizenet Relates to Mage_Authorizenet Component: Bundle Relates to Mage_Bundle Component: Catalog Relates to Mage_Catalog Component: CatalogIndex Relates to Mage_CatalogIndex Component: CatalogInventory Relates to Mage_CatalogInventory Component: CatalogSearch Relates to Mage_CatalogSearch Component: Centinel Relates to Mage_Centinel Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: Cron Relates to Mage_Cron Component: Customer Relates to Mage_Customer Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: lib/Mage Relates to lib/Mage Component: lib/Magento Relates to lib/Magento Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* Component: Media Relates to Mage_Media Component: Newsletter Relates to Mage_Newsletter Component: Paygate Relates to Mage_Paygate Component: Payment Relates to Mage_Payment Component: PayPal Relates to Mage_Paypal Component: ProductAlert Relates to Mage_ProductAlert Component: Rss Relates to Mage_Rss Component: Sales Relates to Mage_Sales Component: SalesRule Relates to Mage_SalesRule Component: Tax Relates to Mage_Tax Component: Widget Relates to Mage_Widget Component: Wishlist Relates to Mage_Wishlist environment rector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants